-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Instanciate new empty lists for each instance of WorkflowRepoManager #415
base: main
Are you sure you want to change the base?
Instanciate new empty lists for each instance of WorkflowRepoManager #415
Conversation
Previously we were assigning values to attributes on the class level. For list in particular, this had the side effect of leaking values between instances: ``` >>> m = OwnersFileSubmissionsE2ETest() >>> m.repo_manager._WorkflowRepoManager__local_branches_created ['e2e-owners-d376dfa-6d3820eb5c624ddc9066f96f1a69640e'] >>> m.cleanup() >>> m.repo_manager._WorkflowRepoManager__local_branches_created [] >>> m = OwnersFileSubmissionsE2ETest() >>> m.repo_manager._WorkflowRepoManager__local_branches_created ['e2e-owners-d376dfa-6d3820eb5c624ddc9066f96f1a69640e', 'e2e-owners-d376dfa-cc7f8acf1850498cb6c758bc286f2bec'] ``` Using a dataclass for WorkflowRepoManager, we can ensure that empty lists are initialized for each new instance, and attributes values remain independant. Fix openshift-helm-charts#414 Signed-off-by: Matthias Goerens <[email protected]>
1de8bf0
to
d5af3a5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you know, I'm not a particularly huge fan of dataclasses in class definitions that have a substantial amount of logic. With that said, I think this may solve the problem, but I still feel it changes the contract of the class in a way that may not be strictly required.
The issue with WorkflowRepoManager causing this bug is that I heavily used class attributes instead of instance attributes. I did some reading, and this is bug seems to be caused strictly by assumptions I made in how attributes should be defined.
I think the following solves this in the exact same way, without the use of fields or dataclasses. We're not using any additional features of fields and dataclasses, so I'd advocate against using them here.
class WorkflowRepoManager:
def __init__(self):
logging.debug(f"{self} --> __init__ called!")
# Keep a log of things created so we can clean them up.
self.__local_branches_created: list[str] = []
self.__local_worktrees_created: list[TemporaryDirectory] = []
# (remote, branch), e.g. ('openshift-helm-charts/charts, 'my-pr-branch')
self.__remote_branches_created: list[tuple[str, str]] = []
# The token to use for GitHub API operations.
self.__authtoken: str = ""
# Working directory at instantiation.
self.old_cwd: str = os.getcwd()
# The repository at working directory.
try:
self.repo = git.Repo()
except git.InvalidGitRepositoryError as e:
raise RepoManagementError(
"Unable to initialize git repository. Is the current directory a git repo?"
) from e
# The branch at repo initialization. On Cleanup, we return to this branch
# before we remove locally generated branches.
try:
self.original_branch = self.repo.active_branch.name
except TypeError:
self.original_branch = self.repo.git.rev_parse("--short", "HEAD")
...
In other words, if I had just instantiated all of the variables in __init__(...)
, we probably wouldn't have this buggy behavior. My apologies.
Let me know what you think.
# (remote, branch), e.g. ('openshift-helm-charts/charts, 'my-pr-branch') | ||
__remote_branches_created: [(str, str)] = [] | ||
__remote_branches_created: list[(str, str)] = field(default_factory=list) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
__remote_branches_created: list[(str, str)] = field(default_factory=list) | |
__remote_branches_created: list[tuple[str, str]] = field(default_factory=list) |
Previously we were assigning values to attributes on the class level. For list in particular, this had the side effect of leaking values between instances:
Using a dataclass for WorkflowRepoManager, we can ensure that empty lists are initialized for each new instance, and attributes values remain independant.
Fix #414